Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ConfiguredTargetFunction.computeUnloadedToolchainContexts to #13258

Closed
wants to merge 3 commits into from

Conversation

katre
Copy link
Member

@katre katre commented Mar 22, 2021

directly require the correct execution platform.

This fixes several issues around toolchains that depend on toolchains.

Fixes #13243.

directly require the correct execution platform.

This fixes several issues around toolchains that depend on toolchains.

Fixes bazelbuild#13243.
@katre katre requested a review from gregestren March 22, 2021 18:59
@google-cla google-cla bot added the cla: yes label Mar 22, 2021
@@ -520,18 +520,31 @@ public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunc

Map<String, ToolchainContextKey> toolchainContextKeys = new HashMap<>();
String targetUnloadedToolchainContext = "target-unloaded-toolchain-context";
ToolchainContextKey toolchainContextKey;

ToolchainContextKey.Builder toolchainContextKeyBuilder =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble keeping track of all the interoperating pieces.

What precisely does the parent toolchain context mean w.r.t. the current one? That's the toolchain context of the configured target that may or may not exist, depending on which key instantiated ConfiguredTargetFunction?

What's the full logic flow here? What exactly does dropping and loop avoidance do?

I just need a bit more of an overview to contextualize my understanding.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a configured target CT depends on a toolchain T, it also passed CT's ToolchainContextKey along. This allows T to make sure that it uses the same execution platform, to enable T's dependencies to use the exec transition and target the correct platform.

Previously, we were just throwing away T's toolchain context key, but that isn't correct: T may have its own required toolchain types. This change is adding a clearer way, where T can determine directly that CT's execution platform is, and then forcing toolchain resolution to use that (or throw an error, if it's not possible for some reason).

The specific use here is that rules_rust's rust_toolchain rule needs a cc_toolchain, and that was failing with the previous code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I remember more of the general ToolchainContextKey semantics better, thanks. I don't think I'd considered before the idea of a toolchain requiring another toolchain - that's an interesting extension of the functionality.

@katre
Copy link
Member Author

katre commented Mar 23, 2021

Fixed the test failure and cleaned up ToolchainResolutionFunction a bit.

@bazel-io bazel-io closed this in 5593358 Mar 23, 2021
@katre katre deleted the i13243-recursive-toolchains branch March 23, 2021 18:04
philwo pushed a commit that referenced this pull request Apr 19, 2021
directly require the correct execution platform.

This fixes several issues around toolchains that depend on toolchains.

Fixes #13243.

Closes #13258.

PiperOrigin-RevId: 364597996
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Toolchains that require toolchains don't get toolchains
2 participants